Add support for DHCP options set #20
Conversation
There was a problem hiding this comment.
Few comments about naming, also run terraform fmt or use https://github.com/antonbabenko/pre-commit-terraform
main.tf
Outdated
| # DHCP Options Set | ||
| ################### | ||
| resource "aws_vpc_dhcp_options" "this" { | ||
| count = "${ var.enable_dhcp_options ? 1 : 0 }" |
There was a problem hiding this comment.
count = "${var.enable_dhcp_options ? 1 : 0 }"- Add new line after
countargument
main.tf
Outdated
| resource "aws_vpc_dhcp_options" "this" { | ||
| count = "${ var.enable_dhcp_options ? 1 : 0 }" | ||
| domain_name = "${var.dhcp_options_domain_name}" | ||
| domain_name_servers = "${var.dhcp_options_dns_servers}" |
There was a problem hiding this comment.
Rename dhcp_options_dns_servers to dhcp_options_domain_name_servers
main.tf
Outdated
| domain_name = "${var.dhcp_options_domain_name}" | ||
| domain_name_servers = "${var.dhcp_options_dns_servers}" | ||
| ntp_servers = "${var.dhcp_options_ntp_servers}" | ||
| netbios_name_servers = "${var.dhcp_options_netbios_servers}" |
There was a problem hiding this comment.
Rename dhcp_options_netbios_servers to dhcp_options_netbios_name_servers
main.tf
Outdated
| # DHCP Options Set Association | ||
| ############################### | ||
| resource "aws_vpc_dhcp_options_association" "this" { | ||
| count = "${ var.enable_dhcp_options ? 1 : 0 }" |
There was a problem hiding this comment.
Same as for previous count
variables.tf
Outdated
|
|
||
| variable "dhcp_options_dns_servers" { | ||
| type = "list" | ||
| description = "Specify a list of DNS server addresses for DHCP options set, default to AWS provided" |
There was a problem hiding this comment.
Reorder a bit - description, type, default
|
Hi Anton - Hopefully I addressed all the requested changes - thanks! |
main.tf
Outdated
|
|
||
| vpc_id = "${aws_vpc.this.id}" | ||
| dhcp_options_id = "${aws_vpc_dhcp_options.this.id}" | ||
| depends_on = ["aws_vpc.this", "aws_vpc_dhcp_options.this"] |
There was a problem hiding this comment.
depends_on is not really needed here. Or, if this is this an edge case then provide an open issue link.
There was a problem hiding this comment.
Agreed - no edge case that I know of, I'm not entirely sure why I put that in there. Fixed!
|
Brian, you are rock-star of the Saturday! v1.2.0 is out :) |
* 'master' of github.com:Shapeways/terraform-aws-vpc: Reverted bad merge, fixed terraform-aws-modules#33 Set enable_dns_support=true by default Updated descriptions for DNS variables (closes terraform-aws-modules#14) Add version requirements in README.md (fixes terraform-aws-modules#32) Add version requirements in README.md make sure outputs are always valid (terraform-aws-modules#29) Add tags to the aws_vpc_dhcp_options resource (terraform-aws-modules#30) Add support for DHCP options set (terraform-aws-modules#20) terraform-aws-modules#22 add vpn gateway feature (terraform-aws-modules#24) Add cidr_block outputs to public and private subnets (terraform-aws-modules#19)
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
No description provided.